[BB-803] add missing provisioning for postgres #36
Conversation
WalkthroughThis update introduces grant and revoke functionality for PostgreSQL tables, views, sequences, functions, and procedures, including both implementation and comprehensive tests. It adds or updates supporting methods in connector and client code, expands the test suite with containerized integration tests, and upgrades dependencies for testing, telemetry, and container management. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant TestUtil as Test Utility (Container)
participant Connector as Connector Syncer
participant Client as Postgres Client
participant DB as PostgreSQL
Test->>TestUtil: SetupPostgresContainer()
TestUtil->>DB: Start container, apply init.sql
Test->>Connector: newTestConnector()
Connector->>Client: Connect to DB
Test->>Connector: Sync()
Connector->>DB: Read resources
Test->>Connector: Grant/ Revoke (table/view/sequence/function/procedure)
Connector->>Client: GrantXxx()/RevokeXxx()
Client->>DB: Execute GRANT/REVOKE SQL
Client-->>Connector: Return result
Connector-->>Test: Return grant/revoke result
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
MarcusGoldschmidt
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
♻️ Duplicate comments (3)
pkg/postgres/functions_test.go (1)
12-40: Test implementation is correct but continues the duplication pattern.The test correctly exercises function privilege management using the
Executeprivilege on theget_test_item_countfunction as expected.This test follows the same pattern as
tables_test.goandprocedures_test.gowith identical structure and logic. Please refer to the refactoring suggestions in thetables_test.goreview to address the code duplication across all these test files.pkg/connector/procedure_test.go (1)
44-58:⚠️ Potential issueCritical: Grant and Revoke methods are undefined on the client type.
Same compilation issue as in
table_test.go- theclient.Grantandclient.Revokemethods don't exist on the*inMemoryConnectorClienttype.This is the same issue identified in
table_test.go. The Grant and Revoke methods need to be implemented on the client type before these tests can run.Also applies to: 65-67
🧰 Tools
🪛 golangci-lint (1.64.8)
44-44: client.Grant undefined (type *inMemoryConnectorClient has no field or method Grant)
(typecheck)
pkg/connector/function_test.go (1)
44-58:⚠️ Potential issueCritical: Grant and Revoke methods are undefined on the client type.
Same compilation issue as in the other connector test files - the
client.Grantandclient.Revokemethods don't exist on the*inMemoryConnectorClienttype.This is the same issue identified in
table_test.goandprocedure_test.go. All three connector test files will fail to compile until the Grant and Revoke methods are implemented on the client type.Also applies to: 65-67
🧰 Tools
🪛 golangci-lint (1.64.8)
44-44: client.Grant undefined (type *inMemoryConnectorClient has no field or method Grant)
(typecheck)
🧹 Nitpick comments (12)
pkg/postgres/views_test.go (1)
12-40: Well-structured test with room for enhanced verification.The test covers important scenarios including grant options and edge cases. The containerized setup provides good isolation for integration testing.
Consider adding verification that privileges were actually granted/revoked:
// After grant, verify privilege exists privileges, err := client.GetViewPrivileges(ctx, "public", "test_table_view", container.Role()) require.NoError(t, err) require.Contains(t, privileges, Select.Name()) // After revoke, verify privilege removed privileges, err = client.GetViewPrivileges(ctx, "public", "test_table_view", container.Role()) require.NoError(t, err) require.NotContains(t, privileges, Select.Name())This would ensure the operations actually modify the privilege state, not just execute without errors.
pkg/postgres/procedures_test.go (1)
12-40: Test implementation looks good with opportunities for enhancement.The test correctly covers the basic grant/revoke scenarios for procedures. The structure follows a logical pattern testing both grant options and edge cases.
Consider these enhancements to make the test more robust:
Verify actual privilege state: Query the database to confirm privileges were actually granted/revoked rather than just checking for no errors.
Reduce code duplication: This test pattern is identical across multiple
*_test.gofiles. Consider extracting a generic test helper:+func testGrantRevoke(t *testing.T, grantFunc, revokeFunc func(context.Context, string, string, string, string, bool) error, schema, objectName, privilege string) { + // Common test logic here +}
Add meaningful assertions: The "revoke without grant" scenarios (lines 34-39) should verify they behave correctly when no privilege exists.
Consider test cleanup: Ensure test state doesn't leak between runs by explicitly revoking all test grants in cleanup.
pkg/connector/sequence_test.go (1)
23-23: Remove duplicate error check.There's a redundant
require.NoError(t, err)call on line 23 that duplicates the check on line 22.c1z, err := manager.LoadC1Z(ctx) require.NoError(t, err) -require.NoError(t, err)pkg/connector/table_test.go (1)
22-23: Remove duplicate error handling.There are duplicate
require.NoError(t, err)calls for the same operation.c1z, err := manager.LoadC1Z(ctx) require.NoError(t, err) - require.NoError(t, err)pkg/connector/procedure_test.go (2)
22-23: Remove duplicate error handling.There are duplicate
require.NoError(t, err)calls for the same operation.c1z, err := manager.LoadC1Z(ctx) require.NoError(t, err) - require.NoError(t, err)
13-70: Consider refactoring to reduce code duplication.The test structure is nearly identical to
table_test.goandfunction_test.go. Consider extracting common test logic into a helper function to improve maintainability.Example refactor approach:
func testGrantRevoke(t *testing.T, resourceType ResourceType, resourceName, privilege string) { // Common setup and test logic ctx, syncer, manager, client := newTestConnector(t) // ... shared implementation } func TestGrantRevokeProcedure(t *testing.T) { testGrantRevoke(t, procedureResourceType, "add_test_item", "execute") }🧰 Tools
🪛 golangci-lint (1.64.8)
44-44: client.Grant undefined (type *inMemoryConnectorClient has no field or method Grant)
(typecheck)
65-65: client.Revoke undefined (type *inMemoryConnectorClient has no field or method Revoke)
(typecheck)
pkg/connector/function_test.go (1)
22-23: Remove duplicate error handling.There are duplicate
require.NoError(t, err)calls for the same operation.c1z, err := manager.LoadC1Z(ctx) require.NoError(t, err) - require.NoError(t, err)pkg/connector/view_test.go (1)
43-57: Consider using helper functions for entitlement ID construction.The test manually constructs the entitlement ID string, which could be brittle and inconsistent with the
formatEntitlementIDhelper function available in the same package.Replace the manual string construction with the helper function:
grantResponse, err := client.Grant(ctx, &connectorv2.GrantManagerServiceGrantRequest{ Principal: &connectorv2.Resource{ Id: roleResource.Id, DisplayName: roleResource.DisplayName, }, Entitlement: &connectorv2.Entitlement{ - Id: fmt.Sprintf("entitlement:view:db%s:%d:select:grant", dbId, rId), + Id: formatEntitlementID(&connectorv2.Resource{ + Id: &connectorv2.ResourceId{ + ResourceType: viewResourceType.Id, + Resource: fmt.Sprintf("view:db%s:%d", dbId, rId), + }, + }, "select", true), Resource: &connectorv2.Resource{ Id: &connectorv2.ResourceId{ ResourceType: viewResourceType.Id, Resource: fmt.Sprintf("view:db%s:%d", dbId, rId), }, }, }, })This approach ensures consistency with the helper function and reduces the risk of format mismatches.
pkg/connector/sequence.go (2)
132-169: Fix misleading error message in Grant method.The error message refers to "roles granted" which is confusing in the context of sequence privileges.
if principal.Id.ResourceType != roleResourceType.Id { - return nil, nil, fmt.Errorf("baton-postgres: only users and roles can have roles granted") + return nil, nil, fmt.Errorf("baton-postgres: only users and roles can be granted sequence privileges") }
171-201: Fix misleading error message in Revoke method.Similar to the Grant method, the error message is misleading.
if principal.Id.ResourceType != roleResourceType.Id { - return nil, fmt.Errorf("baton-postgres: only users and roles can have roles granted") + return nil, fmt.Errorf("baton-postgres: only users and roles can have sequence privileges revoked") }pkg/connector/connector_test.go (2)
22-24: Minor: Organize imports consistently.Consider moving the
osandtestingimports into the main import block for better organization."google.golang.org/grpc/credentials/insecure" "google.golang.org/grpc/test/bufconn" - - "os" - "testing" + "os" + "testing"
177-185: Consider enhancing test comprehensiveness.While this test successfully validates that sync completes without errors, it could be more comprehensive given the extensive setup infrastructure. Consider adding assertions to verify:
- Expected PostgreSQL resources (tables, views, functions, etc.) are discovered
- The C1Z file contains the expected data after sync
- Grant/revoke functionality works as mentioned in the PR objectives
Example enhancement:
func TestConnectorFullSync(t *testing.T) { ctx, syncer, localManager, client := newTestConnector(t) err := syncer.Sync(ctx) require.NoError(t, err) + // Verify sync results + c1z, err := localManager.LoadC1File(ctx) + require.NoError(t, err) + + // Check that expected resource types exist + resourceTypes, err := c1z.ListResourceTypes(ctx, &connectorV2.ResourceTypesServiceListResourceTypesRequest{}) + require.NoError(t, err) + require.NotEmpty(t, resourceTypes.List) err = syncer.Close(ctx) require.NoError(t, err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (273)
go.sumis excluded by!**/*.sumand included by nonevendor/dario.cat/mergo/.deepsource.tomlis excluded by!vendor/**and included by nonevendor/dario.cat/mergo/.gitignoreis excluded by!vendor/**and included by nonevendor/dario.cat/mergo/.travis.ymlis excluded by!vendor/**and included by nonevendor/dario.cat/mergo/CODE_OF_CONDUCT.mdis excluded by!vendor/**and included by nonevendor/dario.cat/mergo/CONTRIBUTING.mdis excluded by!vendor/**and included by nonevendor/dario.cat/mergo/LICENSEis excluded by!vendor/**and included by nonevendor/dario.cat/mergo/README.mdis excluded by!vendor/**and included by nonevendor/dario.cat/mergo/SECURITY.mdis excluded by!vendor/**and included by nonevendor/dario.cat/mergo/doc.gois excluded by!vendor/**and included by nonevendor/dario.cat/mergo/map.gois excluded by!vendor/**and included by nonevendor/dario.cat/mergo/merge.gois excluded by!vendor/**and included by nonevendor/dario.cat/mergo/mergo.gois excluded by!vendor/**and included by nonevendor/github.com/Azure/go-ansiterm/LICENSEis excluded by!vendor/**and included by nonevendor/github.com/Azure/go-ansiterm/README.mdis excluded by!vendor/**and included by nonevendor/github.com/Azure/go-ansiterm/constants.gois excluded by!vendor/**and included by nonevendor/github.com/Azure/go-ansiterm/context.gois excluded by!vendor/**and included by nonevendor/github.com/Azure/go-ansiterm/csi_entry_state.gois excluded by!vendor/**and included by nonevendor/github.com/Azure/go-ansiterm/csi_param_state.gois excluded by!vendor/**and included by nonevendor/github.com/Azure/go-ansiterm/escape_intermediate_state.gois excluded by!vendor/**and included by nonevendor/github.com/Azure/go-ansiterm/escape_state.gois excluded by!vendor/**and included by nonevendor/github.com/Azure/go-ansiterm/event_handler.gois excluded by!vendor/**and included by nonevendor/github.com/Azure/go-ansiterm/ground_state.gois excluded by!vendor/**and included by nonevendor/github.com/Azure/go-ansiterm/osc_string_state.gois excluded by!vendor/**and included by nonevendor/github.com/Azure/go-ansiterm/parser.gois excluded by!vendor/**and included by nonevendor/github.com/Azure/go-ansiterm/parser_action_helpers.gois excluded by!vendor/**and included by nonevendor/github.com/Azure/go-ansiterm/parser_actions.gois excluded by!vendor/**and included by nonevendor/github.com/Azure/go-ansiterm/states.gois excluded by!vendor/**and included by nonevendor/github.com/Azure/go-ansiterm/utilities.gois excluded by!vendor/**and included by nonevendor/github.com/Azure/go-ansiterm/winterm/ansi.gois excluded by!vendor/**and included by nonevendor/github.com/Azure/go-ansiterm/winterm/api.gois excluded by!vendor/**and included by nonevendor/github.com/Azure/go-ansiterm/winterm/attr_translation.gois excluded by!vendor/**and included by nonevendor/github.com/Azure/go-ansiterm/winterm/cursor_helpers.gois excluded by!vendor/**and included by nonevendor/github.com/Azure/go-ansiterm/winterm/erase_helpers.gois excluded by!vendor/**and included by nonevendor/github.com/Azure/go-ansiterm/winterm/scroll_helper.gois excluded by!vendor/**and included by nonevendor/github.com/Azure/go-ansiterm/winterm/utilities.gois excluded by!vendor/**and included by nonevendor/github.com/Azure/go-ansiterm/winterm/win_event_handler.gois excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/.gitattributesis excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/.gitignoreis excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/.golangci.ymlis excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/CODEOWNERSis excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/LICENSEis excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/README.mdis excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/SECURITY.mdis excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/backup.gois excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/doc.gois excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/ea.gois excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/file.gois excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/fileinfo.gois excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/hvsock.gois excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/internal/fs/doc.gois excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/internal/fs/fs.gois excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/internal/fs/security.gois excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/internal/fs/zsyscall_windows.gois excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/internal/socket/rawaddr.gois excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/internal/socket/socket.gois excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/internal/socket/zsyscall_windows.gois excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/internal/stringbuffer/wstring.gois excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/pipe.gois excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/pkg/guid/guid.gois excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/pkg/guid/guid_nonwindows.gois excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/pkg/guid/guid_windows.gois excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/pkg/guid/variant_string.gois excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/privilege.gois excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/reparse.gois excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/sd.gois excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/syscall.gois excluded by!vendor/**and included by nonevendor/github.com/Microsoft/go-winio/zsyscall_windows.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/internal/connector/connector.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connector/v2/annotation_raw_id.pb.gois excluded by!**/*.pb.go,!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connector/v2/annotation_raw_id.pb.validate.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connector/v2/connector.pb.gois excluded by!**/*.pb.go,!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connector/v2/event_feed.pb.gois excluded by!**/*.pb.go,!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connector/v2/event_feed.pb.validate.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connector/v2/event_feed_grpc.pb.gois excluded by!**/*.pb.go,!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connector/v2/resource.pb.gois excluded by!**/*.pb.go,!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connector/v2/resource.pb.validate.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connector/v2/resource_grpc.pb.gois excluded by!**/*.pb.go,!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connectorapi/baton/v1/baton.pb.gois excluded by!**/*.pb.go,!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connectorapi/baton/v1/baton.pb.validate.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/reader/v2/sync.pb.gois excluded by!**/*.pb.go,!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/reader/v2/sync.pb.validate.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/cli/commands.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/config/config.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/connectorbuilder/connectorbuilder.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/connectorrunner/runner.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/connectorstore/connectorstore.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/dotc1z/assets.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/dotc1z/c1file.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/dotc1z/diff.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/dotc1z/entitlements.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/dotc1z/grants.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/dotc1z/resouce_types.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/dotc1z/resources.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/dotc1z/sql_helpers.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/dotc1z/sync_runs.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/field/default_relationships.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/field/defaults.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/field/fields.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/retry/retry.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/sdk/empty_connector.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/sdk/version.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/sync/state.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/sync/syncer.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/synccompactor/compactor.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/synccompactor/naive/naive.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/synccompactor/naive/naive_unroll.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/tasks/c1api/full_sync.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/tasks/c1api/manager.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/tasks/local/compactor.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/tasks/local/differ.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/tasks/local/event_feed.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/tasks/local/syncer.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/tasks/tasks.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/types/entitlement/entitlement.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/types/tasks/tasks.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/types/types.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/uhttp/pagination.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/uhttp/transport.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.gois excluded by!vendor/**and included by nonevendor/github.com/containerd/log/.golangci.ymlis excluded by!vendor/**and included by nonevendor/github.com/containerd/log/LICENSEis excluded by!vendor/**and included by nonevendor/github.com/containerd/log/README.mdis excluded by!vendor/**and included by nonevendor/github.com/containerd/log/context.gois excluded by!vendor/**and included by nonevendor/github.com/containerd/platforms/.gitattributesis excluded by!vendor/**and included by nonevendor/github.com/containerd/platforms/.golangci.ymlis excluded by!vendor/**and included by nonevendor/github.com/containerd/platforms/LICENSEis excluded by!vendor/**and included by nonevendor/github.com/containerd/platforms/README.mdis excluded by!vendor/**and included by nonevendor/github.com/containerd/platforms/compare.gois excluded by!vendor/**and included by nonevendor/github.com/containerd/platforms/cpuinfo.gois excluded by!vendor/**and included by nonevendor/github.com/containerd/platforms/cpuinfo_linux.gois excluded by!vendor/**and included by nonevendor/github.com/containerd/platforms/cpuinfo_other.gois excluded by!vendor/**and included by nonevendor/github.com/containerd/platforms/database.gois excluded by!vendor/**and included by nonevendor/github.com/containerd/platforms/defaults.gois excluded by!vendor/**and included by nonevendor/github.com/containerd/platforms/defaults_darwin.gois excluded by!vendor/**and included by nonevendor/github.com/containerd/platforms/defaults_freebsd.gois excluded by!vendor/**and included by nonevendor/github.com/containerd/platforms/defaults_unix.gois excluded by!vendor/**and included by nonevendor/github.com/containerd/platforms/defaults_windows.gois excluded by!vendor/**and included by nonevendor/github.com/containerd/platforms/errors.gois excluded by!vendor/**and included by nonevendor/github.com/containerd/platforms/platform_compat_windows.gois excluded by!vendor/**and included by nonevendor/github.com/containerd/platforms/platforms.gois excluded by!vendor/**and included by nonevendor/github.com/containerd/platforms/platforms_other.gois excluded by!vendor/**and included by nonevendor/github.com/containerd/platforms/platforms_windows.gois excluded by!vendor/**and included by nonevendor/github.com/cpuguy83/dockercfg/LICENSEis excluded by!vendor/**and included by nonevendor/github.com/cpuguy83/dockercfg/README.mdis excluded by!vendor/**and included by nonevendor/github.com/cpuguy83/dockercfg/auth.gois excluded by!vendor/**and included by nonevendor/github.com/cpuguy83/dockercfg/config.gois excluded by!vendor/**and included by nonevendor/github.com/cpuguy83/dockercfg/load.gois excluded by!vendor/**and included by nonevendor/github.com/davecgh/go-spew/LICENSEis excluded by!vendor/**and included by nonevendor/github.com/davecgh/go-spew/spew/bypass.gois excluded by!vendor/**and included by nonevendor/github.com/davecgh/go-spew/spew/bypasssafe.gois excluded by!vendor/**and included by nonevendor/github.com/davecgh/go-spew/spew/common.gois excluded by!vendor/**and included by nonevendor/github.com/davecgh/go-spew/spew/config.gois excluded by!vendor/**and included by nonevendor/github.com/davecgh/go-spew/spew/doc.gois excluded by!vendor/**and included by nonevendor/github.com/davecgh/go-spew/spew/dump.gois excluded by!vendor/**and included by nonevendor/github.com/davecgh/go-spew/spew/format.gois excluded by!vendor/**and included by nonevendor/github.com/davecgh/go-spew/spew/spew.gois excluded by!vendor/**and included by nonevendor/github.com/distribution/reference/.gitattributesis excluded by!vendor/**and included by nonevendor/github.com/distribution/reference/.gitignoreis excluded by!vendor/**and included by nonevendor/github.com/distribution/reference/.golangci.ymlis excluded by!vendor/**and included by nonevendor/github.com/distribution/reference/CODE-OF-CONDUCT.mdis excluded by!vendor/**and included by nonevendor/github.com/distribution/reference/CONTRIBUTING.mdis excluded by!vendor/**and included by nonevendor/github.com/distribution/reference/GOVERNANCE.mdis excluded by!vendor/**and included by nonevendor/github.com/distribution/reference/LICENSEis excluded by!vendor/**and included by nonevendor/github.com/distribution/reference/MAINTAINERSis excluded by!vendor/**and included by nonevendor/github.com/distribution/reference/Makefileis excluded by!vendor/**and included by nonevendor/github.com/distribution/reference/README.mdis excluded by!vendor/**and included by nonevendor/github.com/distribution/reference/SECURITY.mdis excluded by!vendor/**and included by nonevendor/github.com/distribution/reference/distribution-logo.svgis excluded by!**/*.svg,!vendor/**and included by nonevendor/github.com/distribution/reference/helpers.gois excluded by!vendor/**and included by nonevendor/github.com/distribution/reference/normalize.gois excluded by!vendor/**and included by nonevendor/github.com/distribution/reference/reference.gois excluded by!vendor/**and included by nonevendor/github.com/distribution/reference/regexp.gois excluded by!vendor/**and included by nonevendor/github.com/distribution/reference/sort.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/AUTHORSis excluded by!vendor/**and included by nonevendor/github.com/docker/docker/LICENSEis excluded by!vendor/**and included by nonevendor/github.com/docker/docker/NOTICEis excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/README.mdis excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/common.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/swagger-gen.yamlis excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/swagger.yamlis excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/blkiodev/blkio.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/checkpoint/list.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/checkpoint/options.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/client.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/common/id_response.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/container/change_type.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/container/change_types.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/container/commit.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/container/config.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/container/container.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/container/create_request.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/container/create_response.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/container/errors.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/container/exec.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/container/filesystem_change.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/container/health.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/container/hostconfig.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/container/hostconfig_unix.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/container/hostconfig_windows.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/container/network_settings.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/container/options.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/container/port.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/container/stats.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/container/top_response.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/container/update_response.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/container/wait_exit_error.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/container/wait_response.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/container/waitcondition.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/error_response.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/error_response_ext.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/events/events.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/filters/errors.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/filters/parse.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/image/delete_response.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/image/image.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/image/image_history.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/image/image_inspect.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/image/manifest.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/image/opts.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/image/summary.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/mount/mount.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/network/create_response.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/network/endpoint.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/network/ipam.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/network/network.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/plugin.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/plugin_device.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/plugin_env.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/plugin_interface_type.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/plugin_mount.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/plugin_responses.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/registry/authconfig.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/registry/authenticate.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/registry/registry.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/registry/search.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/storage/driver_data.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/strslice/strslice.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/swarm/common.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/swarm/config.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/swarm/container.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/swarm/network.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/swarm/node.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/swarm/runtime.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/swarm/runtime/gen.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/swarm/runtime/plugin.pb.gois excluded by!**/*.pb.go,!vendor/**and included by nonevendor/github.com/docker/docker/api/types/swarm/runtime/plugin.protois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/swarm/secret.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/swarm/service.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/swarm/service_create_response.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/swarm/service_update_response.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/swarm/swarm.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/swarm/task.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/system/info.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/system/runtime.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/system/security_opts.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/time/timestamp.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/types.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/types_deprecated.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/versions/compare.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/volume/cluster_volume.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/volume/create_options.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/volume/list_response.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/volume/options.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/volume/volume.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/api/types/volume/volume_update.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/client/README.mdis excluded by!vendor/**and included by nonevendor/github.com/docker/docker/client/build_cancel.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/client/build_prune.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/client/checkpoint.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/client/checkpoint_create.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/client/checkpoint_delete.gois excluded by!vendor/**and included by nonevendor/github.com/docker/docker/client/checkpoint_list.gois excluded by!vendor/**and included by none
📒 Files selected for processing (27)
go.mod(5 hunks)pkg/connector/connector_test.go(1 hunks)pkg/connector/function.go(1 hunks)pkg/connector/function_test.go(1 hunks)pkg/connector/helpers.go(1 hunks)pkg/connector/procedure.go(1 hunks)pkg/connector/procedure_test.go(1 hunks)pkg/connector/sequence.go(1 hunks)pkg/connector/sequence_test.go(1 hunks)pkg/connector/table.go(1 hunks)pkg/connector/table_test.go(1 hunks)pkg/connector/view.go(1 hunks)pkg/connector/view_test.go(1 hunks)pkg/postgres/client.go(1 hunks)pkg/postgres/databases.go(1 hunks)pkg/postgres/functions.go(4 hunks)pkg/postgres/functions_test.go(1 hunks)pkg/postgres/procedures.go(5 hunks)pkg/postgres/procedures_test.go(1 hunks)pkg/postgres/sequences.go(2 hunks)pkg/postgres/sequences_test.go(1 hunks)pkg/postgres/tables.go(2 hunks)pkg/postgres/tables_test.go(1 hunks)pkg/postgres/views.go(2 hunks)pkg/postgres/views_test.go(1 hunks)pkg/testutil/container.go(1 hunks)pkg/testutil/init.sql(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (11)
pkg/postgres/functions_test.go (2)
pkg/testutil/container.go (1)
SetupPostgresContainer(44-87)pkg/postgres/privileges.go (1)
Execute(106-106)
pkg/postgres/views_test.go (2)
pkg/testutil/container.go (1)
SetupPostgresContainer(44-87)pkg/postgres/privileges.go (1)
Select(100-100)
pkg/postgres/procedures_test.go (2)
pkg/testutil/container.go (1)
SetupPostgresContainer(44-87)pkg/postgres/privileges.go (1)
Execute(106-106)
pkg/postgres/sequences_test.go (2)
pkg/testutil/container.go (1)
SetupPostgresContainer(44-87)pkg/postgres/privileges.go (1)
Select(100-100)
pkg/postgres/sequences.go (1)
pkg/postgres/client.go (1)
Client(98-102)
pkg/postgres/views.go (1)
pkg/postgres/client.go (1)
Client(98-102)
pkg/postgres/tables_test.go (1)
pkg/testutil/container.go (1)
SetupPostgresContainer(44-87)
pkg/postgres/tables.go (1)
pkg/postgres/client.go (1)
Client(98-102)
pkg/connector/connector_test.go (1)
pkg/testutil/container.go (1)
SetupPostgresContainer(44-87)
pkg/postgres/functions.go (1)
pkg/postgres/client.go (1)
Client(98-102)
pkg/postgres/procedures.go (1)
pkg/postgres/client.go (1)
Client(98-102)
🪛 golangci-lint (1.64.8)
pkg/connector/sequence_test.go
44-44: client.Grant undefined (type *inMemoryConnectorClient has no field or method Grant)
(typecheck)
65-65: client.Revoke undefined (type *inMemoryConnectorClient has no field or method Revoke)
(typecheck)
pkg/connector/function_test.go
44-44: client.Grant undefined (type *inMemoryConnectorClient has no field or method Grant)
(typecheck)
65-65: client.Revoke undefined (type *inMemoryConnectorClient has no field or method Revoke)
(typecheck)
pkg/connector/procedure_test.go
44-44: client.Grant undefined (type *inMemoryConnectorClient has no field or method Grant)
(typecheck)
65-65: client.Revoke undefined (type *inMemoryConnectorClient has no field or method Revoke)
(typecheck)
pkg/testutil/container.go
53-53: undefined: testcontainers
(typecheck)
70-70: undefined: pgx
(typecheck)
71-71: undefined: pgx
(typecheck)
pkg/connector/connector_test.go
143-143: client.Validate undefined (type *inMemoryConnectorClient has no field or method Validate)
(typecheck)
🔇 Additional comments (22)
pkg/testutil/init.sql (1)
1-74: Excellent test database schema setup.The initialization script is well-structured and comprehensive, covering all the database objects needed for testing privilege management:
- Tables with proper primary keys and foreign key constraints
- Views for testing view privileges
- Functions and procedures with correct plpgsql syntax
- Sequences for testing sequence privileges
- Appropriate roles and users for privilege testing scenarios
The SQL syntax is correct and the schema design supports thorough testing of the grant/revoke functionality.
pkg/postgres/client.go (1)
12-12: Good addition for code maintainability.The
withGrantOptionsconstant centralizes the WITH GRANT OPTION clause string, reducing duplication across grant methods and improving maintainability.pkg/connector/sequence_test.go (1)
44-58:Details
❌ Incorrect review comment
Critical issue: Grant method not implemented on client.
The static analysis correctly identifies that
client.Grantis undefined on theinMemoryConnectorClienttype.This test will fail to compile because the
Grantmethod is not implemented. Please verify the implementation status:
🏁 Script executed:
#!/bin/bash # Description: Check if Grant and Revoke methods are implemented on the connector client # Expected: Find method implementations or interface definitions # Search for Grant method implementations ast-grep --pattern 'func ($_ *$_) Grant($$$) $$$' # Search for Revoke method implementations ast-grep --pattern 'func ($_ *$_) Revoke($$$) $$$' # Check the inMemoryConnectorClient type definition ast-grep --pattern 'type inMemoryConnectorClient struct { $$$ }' # Look for Grant/Revoke interface definitions rg -A 10 "Grant.*Request.*Response|Revoke.*Request.*Response"Length of output: 26892
Remove incorrect missing-method warning
The
inMemoryConnectorClientin pkg/connector/connector_test.go embeds the GrantManagerServiceClient interface, which provides the Grant(ctx, *GrantManagerServiceGrantRequest) method. Calls toclient.Grant(...)resolve to that embedded interface, so the test will compile and run as written. No implementation changes are required.Likely an incorrect or invalid review comment.
🧰 Tools
🪛 golangci-lint (1.64.8)
44-44: client.Grant undefined (type *inMemoryConnectorClient has no field or method Grant)
(typecheck)
pkg/postgres/sequences_test.go (1)
12-40: Well-structured integration test with comprehensive scenario coverage.The test effectively covers the main scenarios for sequence privilege management:
- Grant/revoke with
isGrantflag variations- Edge case testing (revoke without prior grant)
- Proper container-based test setup
The test logic is sound and follows good integration testing practices.
pkg/connector/table_test.go (1)
44-58:Details
❌ Incorrect review comment
Critical: Grant and Revoke methods are undefined on the client type.
The static analysis correctly identifies that
client.Grantandclient.Revokemethods don't exist on the*inMemoryConnectorClienttype. This will prevent the test from compiling and running.Verify that the Grant and Revoke methods are properly implemented on the client type:
Also applies to: 65-67
🏁 Script executed:
#!/bin/bash # Description: Check if Grant and Revoke methods exist on the connector client types # Expected: Find method definitions or interface implementations echo "Searching for Grant method implementations:" ast-grep --pattern 'func ($_ *$_) Grant($$$) $$$' echo -e "\nSearching for Revoke method implementations:" ast-grep --pattern 'func ($_ *$_) Revoke($$$) $$$' echo -e "\nSearching for inMemoryConnectorClient type definition:" rg -A 10 "type.*inMemoryConnectorClient" echo -e "\nSearching for connector client interface definitions:" rg -A 5 "type.*Client.*interface"Length of output: 26861
Incorrect static analysis:
GrantandRevokeare already provided via embeddingThe
inMemoryConnectorClientstruct inpkg/connector/connector_test.goembeds the generated gRPC interfaceconnectorv2.GrantManagerServiceClient, which declares bothGrantandRevoke. Those methods are thus promoted ontoinMemoryConnectorClientand will resolve correctly in your table tests. You can ignore the earlier “undefined method” warning.Likely an incorrect or invalid review comment.
pkg/connector/view_test.go (1)
13-69: Well-structured integration test for view grant/revoke functionality.The test follows a clear and comprehensive flow:
- Sets up test connector and performs sync
- Loads C1Z file and retrieves necessary resources
- Tests grant operation and validates response
- Tests revoke operation
The test coverage is appropriate and validates the complete grant/revoke cycle.
pkg/connector/sequence.go (1)
132-201: Well-implemented grant and revoke methods for sequences.Both methods follow consistent patterns with proper:
- Principal validation
- ID parsing using helper functions
- Database client retrieval
- Error handling and response construction
The implementation aligns well with the overall architecture.
pkg/postgres/views.go (2)
100-117: Well-implemented GrantView method with proper sanitization.The method follows best practices:
- Extracts logger for debugging
- Sanitizes all identifiers to prevent SQL injection
- Constructs parameterized SQL with grant option handling
- Uses proper error handling
119-137:Details
✅ Verification successful
Verify PostgreSQL syntax for view privilege revocation.
The implementation uses
ON TABLEfor both regular revoke and grant option revoke operations on views. While this might seem inconsistent, it's likely correct as PostgreSQL treats views similar to tables for privilege operations.Please verify that PostgreSQL accepts
REVOKE ... ON TABLEsyntax for views:The rest of the implementation is solid with proper sanitization, logging, and error handling.
🌐 Web query:
Does PostgreSQL support "REVOKE privileges ON TABLE view_name" syntax for revoking privileges on views?💡 Result:
Revoking Privileges on Views in PostgreSQL
Syntax and Capability
PostgreSQL uses the
REVOKEstatement to remove previously granted privileges from database roles (users or groups) for various database objects, including tables and views. The syntax for revoking privileges is:REVOKE [privileges] ON [object] FROM [role];Where
[object]can be a table, view, sequence, etc.[1][2][4]Application to Views
- In PostgreSQL, views are database objects similar to tables regarding privilege management. As such, you use the same
REVOKEsyntax for views as you do for tables.- You can use the following command to revoke privileges on a view:
REVOKE SELECT ON view_name FROM role_name;or, more generally,
REVOKE [SELECT | INSERT | UPDATE | DELETE | ALL] ON view_name FROM role_name;
- The documentation and common practice support the use of
REVOKE ... ON TABLE view_name, because in PostgreSQL, the keywordTABLEin privilege commands can refer to either a physical table or a view. Thus, the following are functionally equivalent:
REVOKE SELECT ON view_name FROM user123;REVOKE SELECT ON TABLE view_name FROM user123;[1][2][4][5]Conclusion
Yes, PostgreSQL supports the syntax
REVOKE privileges ON TABLE view_namefor revoking privileges on views. TheTABLEkeyword in this context applies to both tables and views, and the privileges that can be revoked (such asSELECT,INSERT,UPDATE,DELETE) are appropriate for views as well[1][2][4][5].Example
REVOKE SELECT ON TABLE my_view FROM some_user;This command revokes the
SELECTprivilege on the viewmy_viewfrom the usersome_userin PostgreSQL[1][4].Citations:
- 1: https://www.postgresql.org/docs/current/sql-revoke.html
- 2: https://neon.com/postgresql/postgresql-administration/postgresql-revoke
- 3: https://www.postgresql.org/docs/7.0/sql-revoke.htm
- 4: https://www.datacamp.com/doc/postgresql/revoke
- 5: https://www.postgresql.org/docs/current/ddl-priv.html
- 6: https://materialize.com/docs/sql/revoke-privilege/
- 7: https://docs.progress.com/bundle/openedge-sql-reference/page/Syntax-to-revoke-privileges-on-specific-tables-and-views.html
Confirmed:
REVOKE … ON TABLEIs Valid for Views in PostgreSQLPostgreSQL treats views like tables for privilege management. The
TABLEkeyword inREVOKEstatements applies equally to tables and views, so both of these are equivalent and valid:
REVOKE SELECT ON view_name FROM role_name;REVOKE SELECT ON TABLE view_name FROM role_name;No changes needed—the current implementation in
pkg/postgres/views.gocorrectly usesON TABLEfor revoking view privileges.pkg/postgres/sequences.go (1)
110-127: LGTM! Proper implementation of sequence privilege granting.The method correctly sanitizes all inputs to prevent SQL injection and properly constructs the GRANT statement with optional WITH GRANT OPTION support.
pkg/connector/table.go (2)
142-179: LGTM! Well-structured implementation of table privilege granting.The method properly validates inputs, handles errors appropriately, and correctly delegates to the database client for executing the grant operation.
181-211: LGTM! Consistent implementation of table privilege revocation.The method follows the same pattern as Grant, ensuring consistency in privilege management operations.
pkg/testutil/container.go (1)
44-87:Details
✅ Verification successful
Well-structured test container setup.
The PostgreSQL container setup is well-implemented with appropriate:
- 1-minute timeout for container startup
- Debug logging integration with test framework
- Connection pool configuration with MaxConns=2 for tests
- Embedded SQL initialization script execution
The static analysis hints indicate undefined references, but the imports appear correct. Let me verify the actual availability of these packages:
🏁 Script executed:
#!/bin/bash # Description: Verify the imports are valid and packages are available in go.mod # Check if testcontainers-go is in go.mod echo "Checking testcontainers-go dependency:" cat go.mod | grep testcontainers # Check if pgx is in go.mod echo "Checking pgx dependency:" cat go.mod | grep pgx # Look for any go build or compilation errors echo "Checking for build issues:" go list -e ./pkg/testutil/...Length of output: 509
PostgreSQL test container setup verified and approved
I confirmed that:
github.com/testcontainers/testcontainers-go(including the Postgres module) andgithub.com/jackc/pgx/v4are declared in go.mod.go list -e ./pkg/testutil/...reports no build errors.No further changes are required.
🧰 Tools
🪛 golangci-lint (1.64.8)
53-53: undefined: testcontainers
(typecheck)
70-70: undefined: pgx
(typecheck)
71-71: undefined: pgx
(typecheck)
pkg/postgres/tables.go (2)
119-138: LGTM: RevokeTable method is well-implemented.The RevokeTable method correctly handles both privilege revocation and grant option revocation with proper sanitization and logging.
100-117:Details
✅ Verification successful
Verify the
withGrantOptionsconstant is defined.The GrantTable method implementation looks good with proper sanitization, logging, and SQL construction. However, line 112 references
withGrantOptionswhich should be verified to exist.
🏁 Script executed:
#!/bin/bash # Description: Find the definition of withGrantOptions constant # Search for withGrantOptions definition in the postgres package echo "Searching for withGrantOptions definition:" ast-grep --pattern 'const $_ = $_' | grep -i grant || rg "withGrantOptions.*=" pkg/postgres/ # Alternative search for the constant rg "withGrantOptions" pkg/postgres/ -A 2 -B 2Length of output: 1158
withGrantOptions constant verified and GrantTable implementation approved
The
withGrantOptionsconstant is defined in pkg/postgres/client.go (const withGrantOptions = " WITH GRANT OPTION"), so the reference in GrantTable is valid. The method uses proper sanitization, logging, and SQL construction.No further changes required.
go.mod (1)
6-19: LGTM! Appropriate dependency updates for provisioning support.The dependency updates are well-aligned with the PR objectives:
baton-sdkupgrade to v0.3.9 provides necessary provisioning capabilitiestestcontainerspackages enable PostgreSQL integration testing- OpenTelemetry packages add observability for the new features
pkg/postgres/procedures.go (1)
19-45: Good additions for procedure signature handling.The
Argumentsfield andSignature()method properly support procedure identification. The SQL queries correctly usepg_get_function_arguments()to retrieve the argument list.pkg/postgres/functions.go (1)
19-46: Well-structured additions for function metadata.The
ArgumentsandReturnTypefields along with theSignature()method provide proper support for function identification. The SQL queries correctly use PostgreSQL's built-in functions to retrieve this metadata.pkg/connector/connector_test.go (4)
26-44: LGTM! Well-designed client aggregation.The buffer size and client struct design are appropriate for comprehensive integration testing. The struct effectively aggregates all necessary gRPC service clients.
46-157: Comprehensive test setup with proper resource management.The test infrastructure setup is well-designed with:
- Proper PostgreSQL container initialization
- Complete gRPC server/client setup with OpenTelemetry
- Appropriate cleanup mechanisms using
t.Cleanup()- All necessary service registrations
The approach enables thorough integration testing of the connector.
🧰 Tools
🪛 golangci-lint (1.64.8)
143-143: client.Validate undefined (type *inMemoryConnectorClient has no field or method Validate)
(typecheck)
159-175: LGTM! Clean helper function.The resource lookup logic is correct and handles errors appropriately. The page size of 100 is reasonable for test scenarios.
143-144:Details
❓ Verification inconclusive
Address static analysis error: Validate method undefined.
The static analysis tool reports that
client.Validateis undefined. This suggests theConnectorServiceClientinterface may not have aValidatemethod, or there's a type mismatch.Please verify the correct method signature by checking the connector service definition:
🏁 Script executed:
#!/bin/bash # Check if Validate method exists in ConnectorServiceClient interface ast-grep --pattern 'type ConnectorServiceClient interface { $$$ }' # Also search for any Validate method in connector service definitions rg -A 10 "Validate.*Request"Length of output: 727
🏁 Script executed:
#!/bin/bash # Locate the generated protobuf Go file defining ConnectorServiceClient rg -n "type ConnectorServiceClient" -g "*.pb.go" # Verify if any RPC named Validate exists in the proto definitions rg -n "rpc Validate" -g "*.proto" # If found, show the surrounding lines for context rg -A3 -B3 "rpc Validate" -g "*.proto"Length of output: 574
Verify
client.Validateexists with correct signature
I couldn’t locate the generated gRPC client code (.pb.go) forconnectorV2in this repo—this package is likely external. Please manually confirm thatconnectorV2.ConnectorServiceClientdefines:Validate(ctx context.Context, in *ConnectorServiceValidateRequest, …grpc.CallOption) (*ConnectorServiceValidateResponse, error)If the method name or signature differs, update the test accordingly at:
- pkg/connector/connector_test.go:143-144 (
client.Validatecall)🧰 Tools
🪛 golangci-lint (1.64.8)
143-143: client.Validate undefined (type *inMemoryConnectorClient has no field or method Validate)
(typecheck)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
pkg/connector/function_test.go (1)
13-70: Address code duplication across test files.This test duplicates the exact same pattern found in other connector test files, as identified in previous reviews.
🧰 Tools
🪛 golangci-lint (1.64.8)
44-44: client.Grant undefined (type *inMemoryConnectorClient has no field or method Grant)
(typecheck)
65-65: client.Revoke undefined (type *inMemoryConnectorClient has no field or method Revoke)
(typecheck)
pkg/connector/procedure_test.go (1)
13-70: Address code duplication across test files.This test follows the exact same pattern as
function_test.goand other connector test files, differing only in resource type and names. This creates maintenance overhead and inconsistency risks.🧰 Tools
🪛 golangci-lint (1.64.8)
44-44: client.Grant undefined (type *inMemoryConnectorClient has no field or method Grant)
(typecheck)
65-65: client.Revoke undefined (type *inMemoryConnectorClient has no field or method Revoke)
(typecheck)
pkg/connector/procedure.go (2)
112-114: Fix the error message to match the validation logic.The error message mentions "users and roles" but the validation only allows roles. Also, the message incorrectly refers to "procedure granted" instead of privileges.
153-155: Fix the error message to match the validation logic.Same issue as in the Grant method - the error message mentions "users and roles" but validation only allows roles, and incorrectly refers to "procedure revoked" instead of privileges.
pkg/connector/function.go (2)
130-132: Fix the error message to match the validation logic.The error message mentions "users and roles" but the validation only allows roles. Also, the message incorrectly refers to "function granted" instead of privileges.
171-173: Fix the error message to match the validation logic.Same issue as in the Grant method - the error message mentions "users and roles" but validation only allows roles, and incorrectly refers to "function revoked" instead of privileges.
🧹 Nitpick comments (1)
pkg/postgres/pagination.go (1)
8-9: Consider the impact of the MinPageSize increase.The MinPageSize was increased from 10 to 100 (10x increase), which means even small queries will now fetch at least 100 records. While this may improve efficiency for large datasets, it could increase memory usage and latency for operations that only need a few records.
Consider if this dramatic increase is necessary, or if a more moderate increase (e.g., to 25 or 50) would balance efficiency gains with resource usage for smaller result sets.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
cmd/baton-postgresql/main.gois excluded by none and included by none
📒 Files selected for processing (17)
pkg/config/conf.gen.go(1 hunks)pkg/config/config.go(1 hunks)pkg/connector/connector.go(3 hunks)pkg/connector/connector_test.go(1 hunks)pkg/connector/function.go(4 hunks)pkg/connector/function_test.go(1 hunks)pkg/connector/procedure.go(2 hunks)pkg/connector/procedure_test.go(1 hunks)pkg/connector/sequence.go(1 hunks)pkg/connector/table.go(1 hunks)pkg/connector/view.go(1 hunks)pkg/postgres/databases.go(1 hunks)pkg/postgres/functions.go(5 hunks)pkg/postgres/functions_test.go(1 hunks)pkg/postgres/pagination.go(1 hunks)pkg/postgres/procedures.go(5 hunks)pkg/postgres/procedures_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- pkg/config/conf.gen.go
- pkg/config/config.go
🚧 Files skipped from review as they are similar to previous changes (8)
- pkg/postgres/databases.go
- pkg/postgres/procedures_test.go
- pkg/postgres/functions_test.go
- pkg/connector/sequence.go
- pkg/connector/table.go
- pkg/postgres/functions.go
- pkg/connector/view.go
- pkg/postgres/procedures.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
pkg/connector/connector.go (2)
pkg/postgres/client.go (4)
ClientDatabasesPool(14-21)New(121-148)NewClientDatabasesPool(23-40)WithSchemaFilter(115-119)pkg/config/conf.gen.go (1)
Postgresql(6-13)
pkg/connector/function.go (2)
pkg/postgres/client.go (1)
ClientDatabasesPool(14-21)pkg/postgres/pagination.go (1)
Pager(12-15)
🪛 golangci-lint (1.64.8)
pkg/connector/connector_test.go
144-144: client.Validate undefined (type *inMemoryConnectorClient has no field or method Validate)
(typecheck)
pkg/connector/function_test.go
44-44: client.Grant undefined (type *inMemoryConnectorClient has no field or method Grant)
(typecheck)
65-65: client.Revoke undefined (type *inMemoryConnectorClient has no field or method Revoke)
(typecheck)
pkg/connector/procedure_test.go
44-44: client.Grant undefined (type *inMemoryConnectorClient has no field or method Grant)
(typecheck)
65-65: client.Revoke undefined (type *inMemoryConnectorClient has no field or method Revoke)
(typecheck)
🔇 Additional comments (8)
pkg/connector/connector.go (2)
20-20: Clean implementation of the skipBuiltInFunctions feature.The addition of the
skipBuiltInFunctionsfield is well-implemented and integrates properly with the existing code structure.
65-87: Constructor properly handles the new parameter.The
Newfunction correctly accepts and initializes theskipBuiltInFunctionsparameter, maintaining consistency with the configuration structure and properly passing it to the function syncer.pkg/connector/procedure.go (1)
60-60: LGTM! Improved display name using function signature.Using
o.Signature()instead of just the name provides better clarity for procedures with parameters.pkg/connector/function.go (3)
21-24: LGTM! Good addition of skipBuiltInFunctions configuration.The new
skipBuiltInFunctionsfield provides useful control over which functions are synchronized.
51-51: LGTM! Proper usage of skipBuiltInFunctions flag.The flag is correctly passed to the
ListFunctionsmethod to control filtering.
61-61: LGTM! Improved display name using function signature.Using
o.Signature()provides better clarity for functions with parameters and return types.pkg/connector/connector_test.go (2)
46-158: LGTM! Well-structured integration test setup.The test harness is well-designed with:
- Proper container setup for PostgreSQL
- Complete gRPC server registration
- Appropriate cleanup functions
- Comprehensive client service aggregation
🧰 Tools
🪛 golangci-lint (1.64.8)
144-144: client.Validate undefined (type *inMemoryConnectorClient has no field or method Validate)
(typecheck)
178-186: LGTM! Good basic integration test.The test validates the full sync process and ensures proper cleanup.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores